fix(server): use weak comparison for If-None-Match - #2710
Conversation
commit: |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
# Conflicts: # packages/vinext/src/server/prod-server.ts
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix(server): use weak comparison for If-None-Match
The core change — matchesIfNoneMatch in http-conditional.ts and its adoption in prod-server.ts — is excellent. It's a clean, RFC-9110-correct, linear-time parser, well tested in http-conditional.test.ts, and the weak-comparison fix is right. The serve-static.test.ts update correctly proves strong-vs-weak matching.
My main concern is scope. The PR title and body describe a focused entity-tag matching fix, but the diff also introduces a 551-line dev-server public-file ETag subsystem (dev-public-etag.ts) plus 42 lines of index.ts wiring and ~700 lines of tests for it. That's ~1300 of the 1475 added lines, and the PR description doesn't mention it at all. This is the bulk of the review surface and risk.
Scope / reviewability
- The dev ETag indexing feature (Unicode case-folding, NFD/NFC normalization detection, symlink-cycle traversal, trie path resolution, filesystem case-sensitivity probing) is a substantial, independent feature. Per
AGENTS.md("For large refactors, prefer small stacked PRs" and keeping PRs reviewable), this should be split into its own PR with its own description explaining the motivation (dev/prod parity forIf-None-Matchonpublic/files). As-is, a reviewer approving "a weak-comparison fix" is actually approving a large new dev-server code path. - If the two are genuinely coupled, the PR body should at minimum document the dev-server changes, the platform assumptions (much of the logic is
darwin-gated in tests), and the startup-cost tradeoffs below.
Startup and watcher cost (dev only, but worth confirming)
See inline comments. createDevPublicFileEtags does a synchronous recursive realpathSync.native + statSync walk of the entire public/ tree at dev-server boot, plus detectCaseInsensitiveDirectory/detectNormalizationInsensitiveDirectory probes. And every watcher event that updateDevPublicFileEtag can't handle incrementally triggers a full rescan. For large public/ directories this could add noticeable cold-start latency and per-change churn. Worth a benchmark note or a comment acknowledging the tradeoff.
Correctness (minor)
One fail-closed branch in indexRequestPath looks slightly off (nulls realPath on a redirect conflict rather than the redirect) — see inline. It's safe (fails closed) but the intent should be documented.
Nothing blocking on the actual entity-tag fix — that part is ready. The ask is to separate/document the dev subsystem so it can be reviewed on its own merits.
|
|
||
| configureServer(server: ViteDevServer) { | ||
| const devPublicDir = server.config.publicDir || undefined; | ||
| let devPublicFileEtags = devPublicDir ? createDevPublicFileEtags(devPublicDir) : undefined; |
There was a problem hiding this comment.
createDevPublicFileEtags(devPublicDir) runs synchronously here at dev-server startup and performs a full recursive fs.realpathSync.native + fs.statSync walk of the entire public/ tree, plus two directory case/normalization probes. For apps with large public/ directories this adds cold-start latency on the critical dev-boot path. Consider documenting the tradeoff, or building the index lazily / off the hot path. A benchmark note in the PR body would help reviewers weigh this.
| devPublicDir, | ||
| undefined, | ||
| undefined, | ||
| devPublicFileEtags.viteUsesStatLookup, |
There was a problem hiding this comment.
Any watcher event updateDevPublicFileEtag can't handle incrementally causes a full createDevPublicFileEtags rescan of the whole public tree. With five watcher events wired (add/change/unlink/addDir/unlinkDir), directory-level changes and removals-behind-symlinks will each trigger a complete recursive rescan. For large public/ dirs this could be a lot of repeated sync fs work during active development. Worth confirming this is acceptable or debouncing/narrowing the rebuild trigger.
| if (redirect) { | ||
| if (node.redirect === undefined || node.redirect === redirect) node.redirect = redirect; | ||
| else node.realPath = null; | ||
| } |
There was a problem hiding this comment.
On a conflicting redirect, this nulls node.realPath rather than the redirect itself, leaving node.redirect holding a stale value. It appears intentional (fail closed so the ambiguous node resolves nothing), but it's subtle — a reader would expect the conflicting redirect to be nulled. Please add a comment explaining why realPath is the field poisoned here.
| expandedCaseTokens: string[]; | ||
| }; | ||
|
|
||
| const simpleCasePatterns = new Map<string, RegExp>(); |
There was a problem hiding this comment.
simpleCasePatterns is an unbounded module-level cache. In practice it's bounded by the set of distinct uppercase case-expansion tokens across public filenames, so it won't grow without limit, but a one-line comment noting the effective bound would preempt the "is this a leak?" question for the next reader.
|
Review posted successfully. Here's a summary of my findings on PR #2710: Review SummaryThe core fix is solid. Main issue: scope creep. The PR title/body describe a focused ~88-line entity-tag matching fix, but the diff actually adds a 551-line dev-server public-file ETag subsystem ( Secondary concerns (all dev-only, non-blocking):
I posted this as a single |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(server): use weak comparison for If-None-Match
Reviewed the diff against the true merge base — this PR is exactly the 4 files it claims (+147/-14): http-conditional.ts, prod-server.ts, and the two test files. The static-file-cache.ts deltas that show up in a raw local origin/main diff came from #2713, which is already on main.
Verified locally
vp test run tests/http-conditional.test.ts→ 13 passedvp test run tests/serve-static.test.ts→ 47 passed (60 total, matches the PR body)vp checkon all 4 files → format, lint, type all clean
The fix is correct
matchesIfNoneMatchimplements RFC 9110 weak comparison (stripsW/on both the current tag and each candidate before comparing opaque tags), which is the actual bug fix — a strongIf-None-Matchnow matches the weak ETags this server generates.- The
etagcgrammar (%x21 / %x23-7E / obs-text) is right: it excludes0x20(space),0x22("), and0x7F(DEL), which the malformed-tag tests confirm. - All three
prod-server.tscall sites always pass a well-formed quoted ETag (eitheretagFromFilenameHash→W/"<hash>"or the mtime fallbackW/"<size>-<mtime>"), so the current-tag validation on lines 10–11 never spuriously rejects a real server ETag. - Comma-in-opaque-tag handling and wildcard-in-list rejection are both correct and tested.
Note on the earlier /bigbonk review
The prior automated review flagged a 551-line dev-public-etag.ts subsystem plus index.ts wiring. That code is not in this PR — the final commit d0d7dbb ("refactor(dev): leave public etag handling to Vite") removed it. The earlier review was reading an intermediate state of the branch. All of its concerns (startup walk cost, watcher rescans, the realPath fail-closed branch, the simpleCasePatterns cache) are moot against the current head.
Minor (non-blocking)
One behavioral nuance not covered by a test: within a comma list, matched ||= … keeps parsing after a match, so a valid match followed by a malformed token (e.g. '"asset", garbage') returns false → full 200, rather than honoring the earlier match. This fails conservatively (never a false 304) and the PR body already documents "malformed fields fail conservatively," but it is a subtle divergence from Next.js's fresh (which splits on comma and would still 304). A one-line test pinning this intent would make it explicit for future readers.
Nothing blocking. The entity-tag fix is ready.
|
Review posted. Summary of my findings on PR #2710: Verdict: the fix is correct and readyThe core change is solid. Verified locally:
Key finding — the earlier One minor non-blocking note: within a comma list, a valid match followed by a malformed token returns Posted as a |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(server): use weak comparison for If-None-Match
Reviewed against the true merge base — the diff is exactly the 4 files claimed (+147/-14). The dev-public-etag.ts subsystem flagged by an earlier /bigbonk run was removed in d0d7dbb and is not part of this PR; those concerns are moot.
The fix is correct
Verified locally:
vp test run tests/http-conditional.test.ts→ 13 passedvp checkon all 4 files → format, lint, type clean
matchesIfNoneMatch implements RFC 9110 weak comparison correctly (strips W/ from both the current tag and each candidate before comparing opaque tags), which is the actual bug fix. The etagc grammar (%x21 / %x23-7E / obs-text), comma-in-opaque-tag handling, wildcard-in-list rejection, empty-member tolerance, and strict current-tag validation are all correct. I confirmed all three prod-server.ts call sites always pass a well-formed quoted ETag (etagFromFilenameHash → W/"<hash>" or the W/"<size>-<mtime>" fallback), so the stricter current-tag validation on line 11 never spuriously rejects a real server ETag. The serve-static.test.ts change correctly proves strong-vs-weak matching. The '"match", garbage' case on line 49 already pins the conservative-fail behavior for a valid match followed by a malformed member, so the divergence-from-fresh nuance is documented and tested.
Non-blocking: a second, divergent matcher still exists
See inline comment. This PR replaces the naive split(",") matcher on the static path, but the Pages Router page path keeps its own etagMatches that still uses exactly the split(",") + W/-strip approach this PR argues against. Worth a follow-up so the two conditional paths don't drift (AGENTS.md dev/prod parity guidance). Pre-existing, not introduced here.
Nothing blocking — the entity-tag fix is ready.
|
Review posted successfully. Summary of my findings on PR #2710: Verdict: the fix is correct and readyCore change is solid. Verified locally:
The diff is genuinely focused — exactly the 4 files claimed (+147/-14). The One non-blocking finding I raised inline: There's a second, parallel If-None-Match matcher — Posted as a |
|
/bigbonk review for issues |
|
@james-elicx Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
|
UnknownError: ProviderInitError |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(server): use weak comparison for If-None-Match
Verified independently against the true merge base — the diff is exactly the 4 source/test files claimed (+152/-30). The earlier /bigbonk run's dev-public-etag.ts scope-creep concerns are moot (that code was removed in d0d7dbb).
The fix is correct
matchesIfNoneMatchimplements RFC 9110 weak comparison correctly: it stripsW/from both the current ETag and each candidate before comparing opaque tags, so a strongIf-None-Matchnow matches the weak ETags this server generates. That is the actual bug fix.- The
etagcgrammar (%x21 / %x23-7E / obs-text) is right — it excludes0x20,0x22, and0x7F, confirmed by the malformed-tag tests. - Comma-in-opaque-tag handling, wildcard-in-list rejection, empty-member tolerance, and strict current-tag validation are all correct.
- I traced every producer of the
etagargument:etagFromFilenameHash(W/"<hash>"), the staticW/"<size>-<mtime>"fallback, andgeneratePagesETag(strong"..."). All are well-formed, so the strict current-tag validation on line 11 never spuriously rejects a real server ETag.
Verified locally
tests/http-conditional.test.ts→ 13 passedtests/serve-static.test.ts→ 48 passedtests/pages-page-response.test.ts→ 53 passed
I also probed additional edge cases (adjacent tags without a comma, double W/, trailing comma, whitespace-only field, ,-only field) — all fail closed or behave per RFC as expected.
Non-blocking observations
- The
etagMatches(etag, ifNoneMatch)wrapper flips argument order relative tomatchesIfNoneMatch(ifNoneMatch, etag). The delegation is correct, but the flip is a mild footgun — see inline note. - Behavior for a wildcard is now stricter than the old
ifNoneMatch === "*"check: the new parser requires the*(plus OWS) to consume the whole field, so"*x"no longer matches. That is more RFC-correct, not a regression, but it is a silent behavioral change on the Pages path worth being aware of.
Nothing blocking — the entity-tag fix is ready.
|
Review posted successfully. Summary of my findings on PR #2710: Verdict: the fix is correct and readyCore change is solid. Independently verified:
The diff is genuinely focused — exactly the 4 files claimed (+152/-30). The Two non-blocking observations (posted, not requesting changes):
Posted as a |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(server): use weak comparison for If-None-Match
Verified against the current head a699cec. The diff is the 4 source files + 3 test files claimed. The dev-public-etag.ts subsystem an earlier /bigbonk run flagged as scope creep was removed in d0d7dbb and is genuinely not part of this PR.
The fix is correct
matchesIfNoneMatch in http-conditional.ts implements RFC 9110 weak comparison correctly — it strips W/ from both the current ETag and each candidate before comparing opaque tags, which fixes the reported bug (a strong If-None-Match now matches the weak ETags this server generates, so 304 instead of a full 200).
- The
etagcgrammar on line 59 (%x21 / %x23-7E / obs-text) is right: it excludes0x20,0x22("), and0x7F(DEL), confirmed by the malformed-tag tests. - Comma-in-opaque-tag handling, wildcard-in-list rejection, empty-member tolerance, and the strict current-tag validation are all correct.
- Traced every producer of the
etagargument:etagFromFilenameHash(W/"<hash>"), the staticW/"<size>-<mtime>"fallback (prod-server.ts:699,static-file-cache.ts:160), andgeneratePagesETag(strong"..."). All are well-formed, so the stricter current-tag validation on line 11 never spuriously rejects a real server ETag. - Both Pages Router paths (
pages-page-data.ts:976,pages-page-response.ts) and all three static call sites now delegate to the shared parser — the parity concern from prior reviews is resolved.
Verified locally
vp test run tests/http-conditional.test.ts tests/pages-page-response.test.ts tests/serve-static.test.ts→ 113 passed (matches the PR body)vp checkon all 4 source files → format, lint, type all clean
Non-blocking observation
One subtle semantic narrowing on the wildcard path — see inline. Not a live bug because all callers pass well-formed ETags, but worth a note for future callers.
Nothing blocking. The entity-tag fix is ready.
| const currentOpaqueTag = etag.slice(current.opaqueStart, current.opaqueEnd); | ||
|
|
||
| let index = skipOptionalWhitespace(ifNoneMatch, 0); | ||
| if (ifNoneMatch[index] === "*") { |
There was a problem hiding this comment.
Subtle semantic narrowing vs. the old matchesIfNoneMatchHeader: the wildcard branch is now gated behind the current-ETag validation on lines 10-11, so If-None-Match: * returns false when etag fails parseEntityTag. RFC 9110 defines * as matching whenever a current representation exists, independent of the ETag's exact form, and the old code returned true for * unconditionally.
This is not a live bug — every caller passes a well-formed W/"..." or "..." ETag (verified all producers), so the wildcard path is never reached with an invalid etag. But if a future caller ever passes a non-quoted or malformed ETag, * would silently stop matching. A one-line comment noting that * intentionally requires a valid current ETag here (or hoisting the * check above the current-tag parse) would preempt the question.
|
Review posted successfully. Summary of my findings on PR #2710: Verdict: the fix is correct and readyCore change is solid. Independently verified against current head
One non-blocking inline note: The Posted as a |
2 participants
What changed
If-None-Match, as required by RFC 9110Why
The production static server previously compared entity tags as exact strings. A strong validator and weak validator with the same opaque tag therefore failed to match, causing a full
200response instead of304 Not Modified.The Pages Router page-response path also had a separate comma-splitting matcher. Both paths now share the same validated parser.
Standards and Next.js parity
Weak comparison is symmetric and matches current Next.js behavior. RFC 9110 also permits commas and literal backslashes inside an opaque tag. This PR intentionally follows that grammar for comma-containing tags even though Next.js 16.2.7 currently delegates to a compiled
freshparser that splits every comma. Malformed fields fail conservatively instead of producing a false match.Unsupported static request methods are handled separately by #2714; this PR remains scoped to entity-tag matching.
Impact
Static and buffered Pages Router conditional requests now share correct weak comparison without changing entity-tag case sensitivity. The parser is linear in the header length and only runs when
If-None-Matchis present.Validation
vp test run tests/http-conditional.test.ts tests/serve-static.test.ts tests/pages-page-response.test.ts— 113 passedvp check tests/http-conditional.test.ts tests/serve-static.test.ts tests/pages-page-response.test.ts packages/vinext/src/server/http-conditional.ts packages/vinext/src/server/prod-server.ts packages/vinext/src/server/pages-page-response.tsvp run vinext#build